Skip to content

Do not consider preprocessed section as part of the sketch. #69

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Mar 26, 2021
Merged

Do not consider preprocessed section as part of the sketch. #69

merged 5 commits into from
Mar 26, 2021

Conversation

cmaglie
Copy link
Member

@cmaglie cmaglie commented Feb 22, 2021

Should fix some auto-format issues.

Maybe this should be done on the sketchmapper side?

per1234
per1234 approved these changes Feb 23, 2021
View reviewed changes
Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using Arduino IDE 2.0.0-beta.2-snapshot.f9730ab, auto format was not working reliably (usually not at all) for me. After replacing the Arduino language server bundled with that build with the one from this PR, I am not noticing any unreliability in the auto format.

@cmaglie
Copy link
Member Author

cmaglie commented Feb 23, 2021

@per1234 I've added the autoformat configuration on this branch if you want to give it a try!

per1234
per1234 reviewed Feb 23, 2021
View reviewed changes
handler/handler.go Outdated
// https://github.com/llvm/llvm-project/blob/64d06ed9c9e0389cd27545d2f6e20455a91d89b1/clang-tools-extra/clangd/ClangdLSPServer.cpp#L856-L868
// https://github.com/llvm/llvm-project/blob/64d06ed9c9e0389cd27545d2f6e20455a91d89b1/clang-tools-extra/clangd/ClangdServer.cpp#L402-L404

config := `
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried it out and it works fine for me.

Would it be possible to load the clang-format configuration from a file. Ideally from somewhere like ~/.arduinoIDE/.clang-format or ~/.arduino15/.clang-format to allow the users to easily customize the auto format configuration, as they can do in the Java IDE via ~/.arduino15/formatter.conf?

Would it be possible to copy .clang-format (if the user added one) from the sketch origin folder to the temporary folder used by the language server, then only write the default .clang-format to that folder if a user-provided one isn't present? This allows bundling a formatter configuration with the sketch so it will be done consistently by all collaborators. Not something any beginner cares about, but worth adding for the advanced users if it doesn't require much effort to implement and maintain.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, I'll try to implement it exactly as you described.

@kittaakos
Copy link
Contributor

kittaakos commented Feb 24, 2021 •
edited by cmaglie
Loading 

cmaglie: hi Akos! is there a way to determine the path to the IDE config file from the LS?
cmaglie :-) (straight to the question :D)
kittaakos: Hi Cristian. Let me think.
cmaglie: maybe you can pass me via cmd-line flag?
kittaakos: From the cwd of the Arduino LS process
kittaakos: That will always work, I am thinking about a quick solution.
cmaglie: that it's not the IDE config directory, but the directory of the IDE program
cmaglie: I need the ~/.arduinoIDE/... or something
kittaakos: Ahh. It is always ~/.arduinoIDE/arduino-cli.yaml
cmaglie: on linux... :-)
kittaakos: I mean, it’s the same on macOS, on windows it is %USER_PROFILE%  or what.
kittaakos: Isn’t a thing in go, os.homeDir() or something? I am just asking.
kittaakos: It is not under AppData/Local/Somthing
cmaglie: yes, but I would like to keep those logic outside of the LS
cmaglie: so it's reusable in case, also another question
kittaakos: OK, so it’s a request, and not a question. Did I get this correctly?
cmaglie: do you think that a file in ~/.arduinoIDE/.clang-format is good? I mean from a UX point of view
cmaglie: yes, but I'm not sure (that's why my last question ^^^)
cmaglie: I guess we need some GUI support for that? like a command like "Edit global clang formatting options" that open the file to edit it, at the very minimum
cmaglie: ok let me do it this way: for now I'll put an OPTIONAL command line flag, that specifies the path of that global clang-conf file. If the file is there it will be used otherwise not
cmaglie: I'll let Ubi or RSora decide on the priority of this one on the IDE

These are my ideas, and I always think about the Arduino LS (and the VS Code wrapper around it) as a standalone thing. It should work without IDE2 too. If I were to do the dev on your side, I would do this. These are just recommendations.

  • The Arduino LS looks for a formatter config file under the good old Arduino locations, where the default arduino-cli-config.yaml lives. (Yes, there won't be any files there)
  • The Arduino LS has an additional flag formatterPath (or similar); if specified, accessible, and valid, that file should win.

From this point, the IDE2 should provide the formatterPath path when it starts the LS. The IDE2 checks; whether there is a formatter file in the sketch folder. If so, it passes to the LS. If no, the IDE2 will create one on the fly, next to the IDE2's internal config, and forwards that to the LS.

Why is it useful:

  • Once we figure out how and when to use one god cli-config file, we will also know what to do with the formatter config.
  • And the is also cool because one can still drop the Arduino LS into VS Code. VS Code users will be able to configure formatterPath manually, via settings.json or a command.

Base automatically changed from master to main February 26, 2021 14:36
@kittaakos kittaakos mentioned this pull request Mar 11, 2021
Closed
cmaglie added 4 commits March 25, 2021 16:08
@cmaglie
Do not consider preprocessed section as part of the sketch.
63e2721 
Maybe this should be done on the sketchmapper side?
@cmaglie
Force a different clang-formatter configuration on autoformat
2b471cf 
The configuration is provided just for the time needed for the formatter
to run, afterwards the file is removed. This should not affect users
since the file is created in the temporary folder.
@cmaglie
Use custom .clang-format file in sketch if present
59f0539
@cmaglie
Added -format-conf-path flag
08de1bd
@cmaglie cmaglie force-pushed the do-not-consider-preprocessed-section branch from e8298d2 to 08de1bd Compare March 25, 2021 15:09
@cmaglie cmaglie merged commit 436276b into main Mar 26, 2021
@cmaglie cmaglie deleted the do-not-consider-preprocessed-section branch March 26, 2021 14:50
@per1234 per1234 mentioned this pull request Oct 21, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@per1234 per1234 per1234 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cmaglie @kittaakos @per1234